Sub 1s loading UI #169
Sub 1s loading UI #169
Conversation
…rls-from-db # Conflicts: # build/esbuild-config.ts # package.json # static/scripts/audit-report/audit.ts # static/scripts/audit-report/helpers.ts # yarn.lock
|
Your code is outdated. Pull from head |
|
lmao I'll get there sooner or later with Git 😂 |
feat: call db to retrieve permits instead of parsing github comments
lib/chainlist
Outdated
There was a problem hiding this comment.
Are you sure you're up to date? If you are then you might need to rebase or something because its unlikely that you went out of your way to remove this module.
|
Optimistic rendering time till claimable:
Non-optimistic rendering time till claimable:
|
|
had a nightmare trying to get that submodule added, thats what caused my initial issues it was crashing my gh desktop app and would not pull/update or replace because it existed in the index no matter how many time I removed it from cache and the git file, nightmare but got there in the end |
|
The rpc speed mapping is cool but I think it's a bit overkill pinging that many rpcs just to fetch a balance and allowance lmao especially if it's a mainnet tx, hitting 49 RPCs is nuts, 15 for gnosis is still pretty wild Looking at the privacy statements within extraRpcs.js some of them look pretty scary with all of the data that are collecting esp when you consider the maxi mindset A task could be to read through them and exclude specific rpcs and lean towards those that collect no data or only onchain data and use the extraRPCs in other areas with direct on-chain writes as opposed to just in this case two reads |
As far as I remember when you use a forked anvil instance and send a transaction then it is not mined instantly but waits for a new block from a forked network. Anyway there is also this option since anvil supports hardhat's RPC requiest. |
|
It's not determined by a block being mined as it involves no onchain writes, it's hitting 127.0.0.1:8545 with a read op to getChainId, followed by two reads for balance and allowance. Afaik you can't stop anvil/hardhat from responding to reads but you could obv cause a write op to 'stall' if you disable auto mine |
I see, there is also the |
I just started looking over some of the flags, often updating and changing plus I've forgotten most I think lmao but that might help test the edge case but either way the UI currently loads quicker than the metamask popup so trying to squeeze a tx through while bal and allowance and fetching is very unlikely and if already connected via mm before page load it still shouldn't interfere with the fetching as its two distinct providers for reads and writes |
|
I got it under one second on my branch. Just fixing some details around refactoring the code |
|
As did I without the additional 15 rpc calls however it's a good fallback function to have if writes are timing out or whatever but for two reads it's heavy artillery 😂 |
My plan is to combine both. Unfortunately I did a large refactor here (again) breaking apart large functions into seperate files. I anticipate some more merge conflicts. Let me get mine merged in first and then we can merge yours in? |
|
I was going to merge yours in but I realized that its not stable either due to a combination of my bad commit and the refactoring being incompatible with some pulls getting merged at the same time. Can you get this stable so we can use it for production? I need more time to fix my branch stability. |
|
I'm a little confused as to what you mean by it's not currently stable. I'm seeing no errors and was able to successfully claim on I realise that I'm merging against
|
It seems unstable when I refresh several times and press claim. If you set up a test wallet you can generate real permits with you can test almost the entire flow up until you confirm the transaction. You will see it only works some times. I assume there is an issue with some of the RPCs that need to be taken into account.
I don't think so but you should merge to development. It was my mistake because I briefly set main as the default branch but I realize given the pace of progress, stability might not be 100% so I switched the default back to development. After manual testing we can merge to main which automatically deploys to pay.ubq.fi
You just hit edit on the top right and change the merge target to development. Either way though the intermittent issues I mentioned I saw on your latest commit. |
|
Hitting localhost I generated 10 permits with an open window each, refreshed a few times on each and claimed to completion and all I could see was a little layout shift to the right within the claim box. Using the networkRPCs the claim button flickers when an RPC call fails, specifically |
|
Let me spend some time testing today and I'll be careful to clear cache etc to ensure that everything works as expected. I'll see if I can post proof of the specific error I remember receiving. Thanks for testing! |
fix: continuous deploys
build/esbuild-build.ts
Outdated
| const officialUrls = extraRpcs[networkId].rpcs.filter((rpc) => { | ||
| if (typeof rpc === "string") { | ||
| if (blacklist.includes(rpc)) { | ||
| return ""; |
There was a problem hiding this comment.
That doesn't look right. You should not return a truthy value if you want to filter it out.
| export async function generateMultiERC20Permits() { | ||
| for (let i = 0; i < 5; i++) { | ||
| const url = await generate(true); | ||
| log.ok("Testing URL:"); | ||
| console.log(url); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it makes sense to put this in a local test file?
|
The error that I got is from. 
|
|
This resolves the no network issue. I test by setting the |
| @@ -14,13 +14,13 @@ export const entries = [...typescriptEntries, ...cssEntries]; | |||
| const allNetworkUrls: Record<string, string[]> = {}; | |||
| // this flattens all the rpcs into a single object, with key names that match the networkIds. The arrays are just of URLs per network ID. | |||
|
|
|||
| const blacklist = ["https://xdai-archive.blockscout.com"]; | |||
| const blacklist = ["https://xdai-archive.blockscout.com", "https://gnosis.api.onfinality.io/public"]; | |||
There was a problem hiding this comment.
This blacklist idea doesn't seem like a robust approach. We can merge it in but it should be handled dynamically. I think I implemented this in an unstable manner on my branch, I vaguely recall.
There was a problem hiding this comment.
Since the issue is that the claim button flickers on a failure then finding out why exactly that's happening because of a failed background call would probs be the best way to handle it I guess, otherwise just try{}catch() and don't log the error? seems hacky we could care less if one or two calls have failed
There was a problem hiding this comment.
It's because the CSS/style logic is implemented poorly. I tore it apart in my refactor but didn't get to put it all together in a totally stable way (mostly the other RPC related functions- I believe the styling code was significantly simplified and stable if I recall correctly).
Unfortunately I'm running very low on code time because I'll need to focus on fundraising starting next week. So I'm just going to be focusing on top priority stuff.
There was a problem hiding this comment.
Luckily for me I've got nothing but code time, regretfully it does eventually require that you have review time lmao but I'll keep on pushing forward with things as best I can in the meantime, cheers for the heads up
| @@ -33,8 +33,7 @@ export async function handleNetwork(desiredNetworkId: number) { | |||
| invalidateButton.disabled = true; | |||
| } | |||
|
|
|||
| const network = await web3provider.getNetwork(); | |||
| const currentNetworkId = network.chainId; | |||
| const currentNetworkId = (await web3provider.getNetwork()).chainId; | |||
There was a problem hiding this comment.
My bad I thought I had replied to this, it was not intentional I can only assume it was from my merging of branches and it's been an oversight to spreading it out again
| await insertErc20PermitTableData(permit, provider, symbol, decimals, table); | ||
| } | ||
|
|
||
| export async function fetchTreasury(contractAddr: string, owner: string, provider: JsonRpcProvider) { |
There was a problem hiding this comment.
More typesafe to define the return type up here via
export async function fetchTreasury(contractAddr: string, owner: string, provider: JsonRpcProvider): { balance: BigNumber; allowance: BigNumber } {
There was a problem hiding this comment.
I'll catch this and the rest in the sync ERC pr
There was a problem hiding this comment.
Something I wasn't able to produce before, spamming refresh there are a few failed calls maybe 3 or 4 and the balance and allowance render as 0, I'll see if I can get that too
Resolves #168
The issue with things as far as I could see was that the UI rendering was being held back due to the laggy RPC calls and you'd see this:
hold_my_brewery.mp4
Although using the static data available through the permit and hardcoded token info we can see this instead:
hold_my_beer.mp4
It has involved moving around a few parts but the new interval for the loading ui rendering is sub 1s
With a forked foundry instance, I was trying to replicate the stalling offline to see if I could squeeze a tx through while the stalling rpc calls takes its time but I'm not able to replicate it and work with real permits as local anvil hits instantly everytime.
But thinking about it, you should be able to as mm will be using a different rpc/provider instance so long as the it's not locking down the UI and a user can click they should be able to make a claim with or without the treasuryInfo